Skip to content

Conversation

@jhfrontz
Copy link

@jhfrontz jhfrontz commented Aug 1, 2025

The decrypt() routines mistakenly use the length of the plaintext buffer for determining the amount of data in the ciphertext buffer. On x86_64, a sufficiently large plaintext buffer will cause a segv/abort (see added tests). In other cases, it will silently overrun the ciphertext end and decrypt garbage.

This fix changes both decrypt() routines to use the ciphertext length instead.

@apoelstra
Copy link
Member

utack 9c0ccd4 but needs reformatting

@jhfrontz
Copy link
Author

jhfrontz commented Aug 3, 2025

@apoelstra I was confused about the formatting failure -- I had followed the existing formatting style. Should I deviate from it for this commit or re-format the entire routine to conform?

@apoelstra
Copy link
Member

@jhfrontz the CI job shows formatting failures only in the assertions in the tests that you added. Just change those to match what the CI wants.

@apoelstra
Copy link
Member

You are welcome to rant about rustfmt being a POS. If it annoys you too much we can switch to a model where we have a weekly cronjob that "fixes" the formatting, or to a model where we have no CI format checks, though I prefer to be running the tool regularly so that we don't need to discuss formatting changes in PRs.

But it is extremely unlikely that there is any combination of options that would lead to it enforcing a "consistent" style.

(BTW, the current source of "inconsistency" is that it has a line-length threshold at which it expects lines to be broken up. We could try setting that threshold to 0 or 100000, but there are multiple thresholds and the tool has built-in limits on how long a line it can read, and some thresholds are configurable only on a nightly compiler, and no matter what it won't leave meticulously-arranged byte arrays or vertically-aligned comments alone.)

@delta1
Copy link
Member

delta1 commented Aug 4, 2025

@jhfrontz i think you just need to run cargo fmt —all

@jhfrontz jhfrontz force-pushed the fix_decrypt_length branch from 9c0ccd4 to e4b9630 Compare August 4, 2025 13:38
@jhfrontz
Copy link
Author

jhfrontz commented Aug 4, 2025

Ah, I didn't notice that the length was short enough to satisfy "fits on one line". Fixed.

@philipr-za philipr-za merged commit 4082ae5 into ElementsProject:main Aug 4, 2025
3 checks passed
@jhfrontz jhfrontz deleted the fix_decrypt_length branch August 4, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants